FIX: Use the invariant culture for capitalization when generating code #2156

K-Tone · 2025-03-27T09:39:52Z

Description

A customer with the Turkish locale set, noticed that when we're generating function names for Input Actions, we're using the wrong function. This way, the "info" input action results in Onİnfo method name (notice the dot above the I, it's apparently how the capital for i looks like in Turkish).

Testing status & QA

Pending a QA pass, local testing by dev, a new regression test added

Overall Product Risks

Complexity: Medium
Halo Effect: Medium

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

ekcoh · 2025-04-02T06:50:04Z

Glanced at the draft PR and I would just like to suggest to (if possible) get some unit test in that could catch any problems with culture info.

K-Tone · 2025-04-02T09:46:50Z

Also I see that some tests are failing due to compilation issues apparently, fixing that in a moment.

ritamerkl

The changes look good to me. Many thanks for going through all the cases!

The PR is still a draft, so this is probably still in work, but just a changelog and a link to a Jira ticket is missing in my POV.

K-Tone · 2025-04-08T12:18:11Z

Moving this out of the draft stage:

Added a new editor regression test precisely for the user issue (Hope Håkan is ok with it)
Looked through the current coverage on this branch and made sure that most of the changed lines in this PR are covered by existing tests

ekcoh

Great to get this fix in, I am approving but had some minor comments I let you decide whether they needs to be addressed or not.

ekcoh · 2025-04-09T06:41:42Z

Assets/Editor/AddScenesToBuild.cs

@@ -66,7 +67,7 @@ private static bool IsPathInExcludedFolder(string path)
        // Check if the path or any part of it contains any of the excluded folder names
        foreach (string folder in excludedFolders)
        {
-            if (path.ToLower().Contains(folder.ToLower()))
+            if (path.Contains(folder, StringComparison.InvariantCultureIgnoreCase))


Great to fix case issues here, but I wonder if this wasn't incorrect to begin with. Shouldn't this be using https://learn.microsoft.com/en-us/dotnet/api/system.io.path?view=net-9.0 which should take platform-specific behavior of underlying file systems into account instead? Its probably fine, but it is a bit of a warning flag in my eyes when code tries to "outsmart" the filesystem implementation. Its also interesting previous code was doing ToLower() on a constant that is already in lowercase (its not an arbitrary input). The point I am after is that on some OS you may have both a XR, Xr, xR and xr directory and they are all different. (e.g. linux)

I guess for paths, we should probably use https://github.com/lucasmeijer/NiceIO because it makes pathing so easily and cool. For this instance, I would agree it might have been somewhat incorrect on case-sensitive file systems - which are normally Linux and the default Mac. Won't target changing behaviour here though.

Assets/Tests/InputSystem.Editor/InputActionAssetManagerTests.cs

ekcoh · 2025-04-09T06:47:43Z

Packages/com.unity.inputsystem/InputSystem/Editor/InputSystemPluginControl.cs

@@ -73,10 +73,10 @@ public static void RegisterPlatform(BuildTarget target)
        private static bool IsPluginInstalled()
        {
            var registeredPackages = UnityEditor.PackageManager.PackageInfo.GetAllRegisteredPackages();
-            var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString().ToLower();
+            var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString();


It looks like you fixed a separate issue here right? Or are active build targets package names ToUpper? I am confused around how this previously was supposed to work. The new code looks solid though, effectively ignoring case.

ekcoh · 2025-04-09T06:56:16Z

Packages/com.unity.inputsystem/InputSystem/Utilities/InternedString.cs

        }

        public static bool operator==(string a, InternedString b)
        {
-            return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase,
-                StringComparison.InvariantCultureIgnoreCase) == 0;
+            return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) == 0;


Minor: One could call the other operator, e.g. return (b == a) to avoid the redundant code

ekcoh · 2025-04-09T06:56:53Z

Packages/com.unity.inputsystem/InputSystem/Utilities/InternedString.cs

        }

        public static bool operator!=(string a, InternedString b)
        {
-            return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase,
-                StringComparison.InvariantCultureIgnoreCase) != 0;
+            return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) != 0;


Minor: One could call the other operator, e.g. return (b != a)

…ration

… the current culture

Pauliusd01 · 2025-04-10T11:12:37Z

Did not forget about this one, will check today/tomorrow

Pauliusd01

LGTM, I checked generating and using input actions with non latin characters (Chinese, Russian, Japanese, Arabic, Hebrew, Korean, Thai, Vietnamese, Turkish) on Windows and Mac with Chinese, English and Turkish locales.

But I have to note that I wasn't able to repro the original turkish i character issue on my end. The latin letters i and I stayed the same after generating them on a different locale

ritamerkl approved these changes Apr 2, 2025

View reviewed changes

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 377cefa to 96e30ba Compare April 3, 2025 14:31

K-Tone marked this pull request as ready for review April 8, 2025 12:16

K-Tone requested a review from Pauliusd01 April 8, 2025 13:41

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 58720fc to e1de946 Compare April 9, 2025 06:01

ekcoh approved these changes Apr 9, 2025

View reviewed changes

K-Tone added 9 commits April 10, 2025 13:28

use invariant culture when capitalizing for the purposes of code gene…

3d05994

…ration

use invariant string operations where possible

12d1278

some more lowercase fun

72ee68e

fix up a compilation issue

052c5dd

format code

09987dc

update changelog

5d88031

Added a regression test for type names that priorly were dependant on…

e9f8a63

… the current culture

fix formatting

9f8740c

some final tweaks after code review

ee83350

K-Tone force-pushed the anthony/ixsb-1406-fix-actions-capitalization branch from 21c0680 to ee83350 Compare April 10, 2025 10:28

Pauliusd01 approved these changes Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Use the invariant culture for capitalization when generating code #2156

FIX: Use the invariant culture for capitalization when generating code #2156

K-Tone commented Mar 27, 2025 •

edited

Loading

ekcoh commented Apr 2, 2025

K-Tone commented Apr 2, 2025

ritamerkl left a comment

K-Tone commented Apr 8, 2025 •

edited

Loading

ekcoh left a comment

ekcoh Apr 9, 2025

K-Tone Apr 9, 2025

ekcoh Apr 9, 2025

ekcoh Apr 9, 2025

ekcoh Apr 9, 2025

Pauliusd01 commented Apr 10, 2025

Pauliusd01 left a comment

FIX: Use the invariant culture for capitalization when generating code #2156

Are you sure you want to change the base?

FIX: Use the invariant culture for capitalization when generating code #2156

Conversation

K-Tone commented Mar 27, 2025 • edited Loading

Description

Testing status & QA

Overall Product Risks

Checklist

ekcoh commented Apr 2, 2025

K-Tone commented Apr 2, 2025

ritamerkl left a comment

Choose a reason for hiding this comment

K-Tone commented Apr 8, 2025 • edited Loading

ekcoh left a comment

Choose a reason for hiding this comment

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

K-Tone Apr 9, 2025

Choose a reason for hiding this comment

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

ekcoh Apr 9, 2025

Choose a reason for hiding this comment

Pauliusd01 commented Apr 10, 2025

Pauliusd01 left a comment

Choose a reason for hiding this comment

K-Tone commented Mar 27, 2025 •

edited

Loading

K-Tone commented Apr 8, 2025 •

edited

Loading